-
-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changed copyrights from SIL International to SIL Global #1350
Conversation
Also added PublicAPI attribute to a lot of public members not used internally to make ReSharper happier. Did a bit of additional refactoring, code cleanup and fixing typos.
Included small amount of minor refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it doesn't hurt anything, but the PublicAPI annotations everywhere feel like a lot of clutter, IMO.
If we were principled in applying it across the board where we think/know these are things clients want to or might want to use, it would be different. But I don't like just adding it to make the IDE happy. There's probably some global setting which can be modified instead?
The worst is when you use a libpalaso package and suddenly have a new dependency on the Jetbrains.Annotations package. I believe there is a setting to say this dependency should not be passed on to clients. (Is it PrivateAssets="all"
?) That should be used everywhere for the Annotations package if we are going to keep using it.
Reviewed 136 of 155 files at r1, 110 of 110 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)
SIL.Core.Desktop/UsbDrive/Linux/UsbDriveInfoUDisks2.cs
line 2 at r2 (raw file):
#if !NETSTANDARD // Copyright (c) 2024 SIL Global
Odd to have the copyright inside conditional code.
Yes, it's PrivateAssets="all". I was under the impression that that package
always got added that way, but if not, I should definitely fix that. There
probably is a setting to just turn off that complaint in the IDE. I'll also
look into that. (Though, of course, technically it is possible for public
members to be unintentionally public, so the alert isn't necessarily bad.)
…On Thu, Oct 24, 2024, 5:35 PM andrew-polk ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I suppose it doesn't hurt anything, but the PublicAPI annotations
everywhere feel like a lot of clutter, IMO.
If we were principled in applying it across the board where we think/know
these are things clients want to or might want to use, it would be
different. But I don't like just adding it to make the IDE happy. There's
probably some global setting which can be modified instead?
The worst is when you use a libpalaso package and suddenly have a new
dependency on the Jetbrains.Annotations package. I believe there is a
setting to say this dependency should not be passed on to clients. (Is it
PrivateAssets="all"?) That should be used everywhere for the Annotations
package if we are going to keep using it.
Reviewed 136 of 155 files at r1, 110 of 110 files at r2, all commit
messages.
*Reviewable
<https://reviewable.io/reviews/sillsdev/libpalaso/1350#-:-OA-O3ZMDDcHVEdqNK5b:b9zcp9l>*
status: all files reviewed, 1 unresolved discussion (waiting on @tombogle
<https://github.com/tombogle>)
------------------------------
*SIL.Core.Desktop/UsbDrive/Linux/UsbDriveInfoUDisks2.cs line 2 at r2
<https://reviewable.io/reviews/sillsdev/libpalaso/1350#-OA-P2jbDvE8jTcyGyxd:-OA-P2jbDvE8jTcyGyxe:b9ymyr8>
(raw file
<https://github.com/sillsdev/libpalaso/blob/fe8c1bc8b0e48a22f23a162f2e5d2fc0126a8206/SIL.Core.Desktop/UsbDrive/Linux/UsbDriveInfoUDisks2.cs#L2>):*
#if !NETSTANDARD// Copyright (c) 2024 SIL Global
Odd to have the copyright inside conditional code.
—
Reply to this email directly, view it on GitHub
<#1350 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAITXRGZHEBUQKHTQRR6JDZ5FRYPAVCNFSM6AAAAABPWWBCT2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOJTG42DKMBSGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…ects to which that dependency had been added Dealt with other review feedback and a build warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tombogle)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Reviewable status: 247 of 248 files reviewed, all discussions resolved (waiting on @tombogle)
SIL.Core.Desktop/UsbDrive/Linux/UsbDriveInfoUDisks2.cs
line 2 at r2 (raw file):
Previously, andrew-polk wrote…
Odd to have the copyright inside conditional code.
True. But that's how it was. Anyway, I've changed it.
Also added PublicAPI attribute to a lot of public members not used internally to make ReSharper happier. Did a bit of additional refactoring, code cleanup and fixing typos.
This change is